airbyte_cdk/manifest_server/routers/manifest.py (1)

186-204: Bare re-raise is fine; add intent comment?

Keeping parity with earlier feedback, would you add a brief comment clarifying the intentional bare raise? wdyt?

-    except HTTPException:
-        # Re-raise HTTPExceptions as-is (like the catalog None check above)
-        raise
+    except HTTPException:
+        # Re-raise HTTPExceptions as-is (like the catalog None check above)
+        # Intentional bare raise: preserve original status and message
+        raise

unit_tests/manifest_server/routers/test_manifest.py (1)

539-657: Add a sanitization test by patching filter_secrets?

To ensure secret scrubbing is exercised, could we patch the router’s filter_secrets to verify it’s invoked and controls the response? For example:

+@patch("airbyte_cdk.manifest_server.routers.manifest.filter_secrets", return_value="<<sanitized>>")
+@patch("airbyte_cdk.manifest_server.routers.manifest.ManifestCommandProcessor")
+@patch("airbyte_cdk.manifest_server.routers.manifest.build_source")
+def test_check_sanitization(
+    self, mock_build_source, mock_runner_class, mock_filter, sample_manifest, sample_config, mock_source
+):
+    mock_build_source.return_value = mock_source
+    mock_runner = Mock()
+    mock_runner.check_connection.side_effect = RuntimeError("secret-token-123")
+    mock_runner_class.return_value = mock_runner
+    response = client.post("/v1/manifest/check", json={"manifest": sample_manifest, "config": sample_config})
+    assert response.status_code == 400
+    assert response.json()["detail"] == "<<sanitized>>"

This validates the sanitization pathway without coupling to secret registration internals, wdyt?

airbyte_cdk/manifest_server/routers/manifest.py (6)

70-76: Document 422 validation responses too?

Since Pydantic validation returns 422s, should we add 422 in responses for completeness (applies to all endpoints)? wdyt?

 @router.post(
     "/test_read",
     operation_id="testRead",
     responses={
-        400: {"description": "Bad Request - Error processing request", "model": ErrorResponse}
+        400: {"description": "Bad Request - Error processing request", "model": ErrorResponse},
+        422: {"description": "Validation error"},
     },
 )

---

121-137: Consider logging and/or simplifying wrapping.

We create AirbyteTracedException but only use its message; would it be simpler to filter_secrets(str(exc)) directly, or alternatively log the full traced error (logger.exception or error.emit_message) for observability? wdyt?

-    except Exception as exc:
-        error = AirbyteTracedException.from_exception(
-            exc, message=f"Error reading stream: {str(exc)}"
-        )
-        # Filter secrets from error message before returning to client
-        sanitized_message = filter_secrets(error.message or str(exc))
-        raise HTTPException(status_code=400, detail=sanitized_message)
+    except Exception as exc:
+        # Optional: logger.exception("test_read failed")  # keep server-side trace
+        sanitized_message = filter_secrets(f"Error reading stream: {str(exc)}")
+        raise HTTPException(status_code=400, detail=sanitized_message)

If you prefer the traced exception for future use, maybe emit/log it instead, wdyt?

---

156-167: Preserve existing HTTPExceptions (consistency with discover).

To keep explicit HTTP statuses/messages from safe_build_source (or future checks), shall we re-raise HTTPException before the generic catch (like discover does)? wdyt?

 def check(request: CheckRequest) -> CheckResponse:
@@
-    try:
+    try:
         source = safe_build_source(request.manifest.model_dump(), request.config.model_dump())
         runner = ManifestCommandProcessor(source)
         success, message = runner.check_connection(request.config.model_dump())
         return CheckResponse(success=success, message=message)
+    except HTTPException:
+        # Preserve explicit HTTP errors raised upstream
+        raise
     except Exception as exc:
-        error = AirbyteTracedException.from_exception(
-            exc, message=f"Error checking connection: {str(exc)}"
-        )
-        # Filter secrets from error message before returning to client
-        sanitized_message = filter_secrets(error.message or str(exc))
+        sanitized_message = filter_secrets(f"Error checking connection: {str(exc)}")
         raise HTTPException(status_code=400, detail=sanitized_message)

---

223-232: Resolve: preserve HTTPExceptions like discover/check.

For consistency and to avoid masking explicit statuses, shall we re-raise HTTPException here too? wdyt?

 def resolve(request: ResolveRequest) -> ManifestResponse:
@@
-    try:
+    try:
         source = safe_build_source(request.manifest.model_dump(), {})
         return ManifestResponse(manifest=Manifest(**source.resolved_manifest))
+    except HTTPException:
+        raise
     except Exception as exc:
-        error = AirbyteTracedException.from_exception(
-            exc, message=f"Error resolving manifest: {str(exc)}"
-        )
-        # Filter secrets from error message before returning to client
-        sanitized_message = filter_secrets(error.message or str(exc))
+        sanitized_message = filter_secrets(f"Error resolving manifest: {str(exc)}")
         raise HTTPException(status_code=400, detail=sanitized_message)

---

255-280: Avoid mutating source.resolved_manifest; preserve HTTPExceptions; tiny wording nit.

full_resolve currently shallow-copies manifest and then mutates streams in-place, which can side-effect source.resolved_manifest across requests. Shall we deep copy before mutation, and also re-raise HTTPException for consistency? Also, consider “fully resolving” wording (optional) to read better. wdyt?

+import copy
@@
-        source = safe_build_source(request.manifest.model_dump(), request.config.model_dump())
-        manifest = {**source.resolved_manifest}
+        source = safe_build_source(request.manifest.model_dump(), request.config.model_dump())
+        manifest = copy.deepcopy(source.resolved_manifest)
         streams = manifest.get("streams", [])
         for stream in streams:
             stream["dynamic_stream_name"] = None
@@
-        return ManifestResponse(manifest=Manifest(**manifest))
+        return ManifestResponse(manifest=Manifest(**manifest))
-    except Exception as exc:
-        error = AirbyteTracedException.from_exception(
-            exc, message=f"Error full resolving manifest: {str(exc)}"
-        )
-        # Filter secrets from error message before returning to client
-        sanitized_message = filter_secrets(error.message or str(exc))
+    except HTTPException:
+        raise
+    except Exception as exc:
+        # Optional: change to "fully resolving" if you want improved grammar
+        sanitized_message = filter_secrets(f"Error full resolving manifest: {str(exc)}")
         raise HTTPException(status_code=400, detail=sanitized_message)

This prevents cross-request mutation and keeps error handling consistent, wdyt?

---

17-26: Minor: centralize exception-to-HTTP conversion?

The try/except + sanitize + raise blocks repeat. Would extracting a small helper (e.g., raise_sanitized_http_400(context, exc)) reduce duplication and keep messages consistent across endpoints? wdyt?

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

airbyte_cdk/manifest_server/routers/manifest.py (1)

109-133: Consider moving safe_build_source inside the try block for consistency.

In test_read, safe_build_source is called at line 109 (before the try block starts at line 120), while in the other endpoints (check, discover, resolve, full_resolve) it's called inside the try block. This means validation errors from safe_build_source will propagate with the original "Invalid manifest" message in test_read, but will be caught and prefixed with "Error checking connection:", etc. in other endpoints.

Is this intentional to preserve the original validation error message in test_read, or should it be moved inside the try block for consistency, wdyt?

Additionally, consider logging the original exception before sanitizing and re-raising to aid debugging:

     runner = ManifestCommandProcessor(source)
     try:
         cdk_result = runner.test_read(
             config_dict,
             catalog,
             converted_state,
             request.record_limit,
             request.page_limit,
             request.slice_limit,
         )
         return StreamReadResponse.model_validate(asdict(cdk_result))
     except Exception as exc:
+        # Log the original exception for debugging
+        import logging
+        logging.getLogger("airbyte.manifest-server").exception("Error in test_read")
         # Filter secrets from error message before returning to client
         sanitized_message = filter_secrets(f"Error reading stream: {str(exc)}")
         raise HTTPException(status_code=400, detail=sanitized_message)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro